Skip to content

Add Cosmos diagnostics contract design doc#4684

Closed
NaluTripician wants to merge 8 commits into
Azure:mainfrom
NaluTripician:nalutripician/cosmos-diagnostics-contract
Closed

Add Cosmos diagnostics contract design doc#4684
NaluTripician wants to merge 8 commits into
Azure:mainfrom
NaluTripician:nalutripician/cosmos-diagnostics-contract

Conversation

@NaluTripician

Copy link
Copy Markdown
Contributor

Design-first: driver↔SDK Cosmos diagnostics contract + OpenTelemetry mapping

Documentation only. Defines the driver↔SDK diagnostics contract and its OpenTelemetry mapping as a standalone design doc. Contract-first: the append-only capture engine prototyped in #4619 is a deferred optimization, referenced here only conceptually — this PR does not depend on or modify it, and stands alone on main.

DIAGNOSTICS-CONTRACT.md

  • A cheap handle (Arc<DiagnosticsContext>, already on main) + explicit, lazy materialization into three representations — structured metrics, OTel spans, JSON string. No single fixed format; JSON (the costliest step, 3.7µs→6.6µs) is paid only when asked for.
  • An opaque-handle + explicit-materialize FFI boundary for the next-major Go/Python SDKs (never forcing JSON at the boundary).
  • A DiagnosticsLevel that bounds high-cardinality transport telemetry (never collection).
  • A bounded-size guarantee for retry storms.
  • The OTel metrics+traces mapping — operation-level metrics always-on; transport-level detail → traces / level-gated to control time-series cardinality — aligned with azure_core's span-attribute constants (incl. the az.service_request.id dot + private-constant gotchas).
  • The resolved design decisions (handle/materialize surface, FFI lifetime, gating knob, bounded caps, SDK-vs-driver emission split, aggregated span-tree shape).

Notes for reviewers

  • References are tagged [main] (exists today), [contract] (proposed here), [[Prototype] Diagnostics capture engine (Cosmos driver) #4619] (deferred capture-engine prototype), so it's clear what depends on what.
  • No code changes; CosmosResponse::diagnostics() stays non-optional; additive/non-breaking. No CHANGELOG (doc/design-only).
  • A throwaway OTel retroactive-span feasibility spike is kept on a separate branch so this contract doc stands alone.
  • Draft: opened for design review (the associated workflow ends at a human-review gate).

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Design-first (contract-first) deliverable: define the driver<->SDK Cosmos
diagnostics CONTRACT and its OpenTelemetry mapping as a standalone document
on main. Follow-up to the deferred capture-engine prototype (PR Azure#4619),
referenced here conceptually only.

DIAGNOSTICS-CONTRACT.md describes:
- a cheap handle (Arc<DiagnosticsContext>, already on main) plus explicit,
  lazy materialization into three representations - structured metrics,
  OTel spans, and a JSON string (no single fixed format);
- an opaque-handle + explicit-materialize FFI boundary for the next-major
  Go/Python SDKs (never forcing JSON at the boundary);
- a DiagnosticsLevel that bounds high-cardinality transport telemetry
  (never collection);
- a bounded-size guarantee for retry storms;
- the OTel metrics+traces mapping (operation-level always-on; transport-
  level level-gated to control time-series cardinality), aligned with
  azure_core's span-attribute constants; and
- the resolved design decisions.

Documentation only - no code changes, no public API change,
CosmosResponse::diagnostics() stays non-optional, additive/non-breaking.
References to main types are tagged [main]; proposed surface [contract];
deferred capture-engine items [Azure#4619]. No CHANGELOG (doc/design-only).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@NaluTripician

Copy link
Copy Markdown
Contributor Author

Design reads well, and the [main] / [contract] / [#4619] tagging makes the dependency story easy to follow. Two coherence issues surfaced reviewing this next to the compaction implementation (#4683):

§6 / Q4 numbers don't match what #4683 ships

The doc states bounded-size defaults of 64 attempt records / 128 spans / 8 KB string, "first+last-per-region." #4683 ships a default cap of 512, no span or byte cap, keyed on endpoint (not region). Per §4 rule 5 an FFI consumer would size a buffer to 64/128/8 KB and under-allocate ~8× on attempts (with no span/byte bound at all). Worth reconciling the numbers, or explicitly marking them as the target for the deferred engine rather than today's behavior.

"Deferred / OFF" vs. hard dependency

§0/§1/Q1 frame #4619 as a deferred optimization that stays OFF and "is not on main," but #4683 depends on #4619's DiagnosticsSummary (computed before compaction) and must merge after it. Read together the two statements look contradictory. A sentence clarifying "the contract doc doesn't depend on #4619, but the compaction impl in #4683 does" would resolve it.

§7.3 "one child span per attempt"

This becomes unrealizable once compaction elides the middle of a run (and §6 itself caps spans at 128). Might note that the per-attempt span mapping describes the uncompacted view, and a compacted run collapses to a single span + count.

Address self-review on PR Azure#4684:

- Reconcile 6/Q4 bounded-size numbers with what PR Azure#4683 actually ships:
  the per-attempt list is bounded by max_request_diagnostics (default 512,
  min 16), keyed on (region, endpoint, status, sub_status, execution_context),
  first+last-per-run + exact aggregates + a bounded per-run rollup. The old
  64-attempt / 128-span / 8 KB figures are re-labelled as target defaults for
  the not-yet-implemented span/string representations and the deferred engine,
  and FFI buffer sizing (4 rule 5) is told to read the configured cap.
- Clarify deferred/OFF vs hard dependency: the contract doc is standalone on
  main and depends on neither Azure#4619 nor Azure#4683, but the Azure#4683 compaction impl
  reuses Azure#4619's DiagnosticsSummary and must merge after it -- so the
  deferred/OFF language describes the capture engine, not the compaction impl.
- Note in 7.3 that the one-span-per-attempt mapping is the uncompacted view;
  under compaction a run collapses to one span + a repeat count, so emitters
  build the tree from requests() (retained), not request_count().

Documentation only; markdownlint clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@NaluTripician

Copy link
Copy Markdown
Contributor Author

Addressed in 60eeb5b (documentation only):

  • §6 / Q4 numbers — reconciled with what Bound Cosmos diagnostics under retry storms #4683 actually ships. The per-attempt list is bounded by max_request_diagnostics (default 512, min 16), keyed on (region, endpoint, status, sub_status, execution_context) with first+last-per-run + exact aggregates + a bounded per-run rollup. The old 64/128/8 KB figures are now explicitly labelled as target defaults for the not-yet-implemented span/string representations and the deferred engine, the "64 attempt records" figure is marked superseded by the 512 default, and §4 rule 5 now tells FFI consumers to size off the configured cap rather than a hardcoded 64.
  • Deferred / OFF vs hard dependency — added a note up front (§0) clarifying that this contract doc is standalone on main and depends on neither [Prototype] Diagnostics capture engine (Cosmos driver) #4619 nor Bound Cosmos diagnostics under retry storms #4683, while the Bound Cosmos diagnostics under retry storms #4683 compaction impl reuses [Prototype] Diagnostics capture engine (Cosmos driver) #4619's DiagnosticsSummary and must merge after it. So the "deferred / OFF / not on main" language (§1, Q1) describes the capture engine, not the compaction impl.
  • §7.3 one-span-per-attempt — added a note that this is the uncompacted view; under compaction a run collapses to one span + a repeat count, so emitters build the tree from requests() (retained), not request_count().

markdownlint clean.

NaluTripician and others added 2 commits July 6, 2026 11:17
…ntract

Address review nit: clarify that the shipped 512 structured-object record
cap and the 128-span target are independent per-representation caps, not a
single number. A span emitter reaches <=128 spans by collapsing each run to
one span + a repeat count (per 7.3), so span count tracks runs (already
capped), not retained records. Also note PR Azure#4683's guarantee that the
retained attempts and the per-run rollup are drawn from the same cap-bounded
set, so an emitter never sees a retained attempt whose run was omitted.

Documentation only.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@NaluTripician

Copy link
Copy Markdown
Contributor Author

Addressed the round-2 doc nit in 5b5467147:

  • Reconciled the per-representation caps: clarified that the shipped 512 structured-object record cap and the 128-span target are independent per-representation caps, not a single number that must agree. A span emitter reaches ≤128 spans by collapsing each run to one span + a repeat count (per §7.3), so span count tracks the number of runs (already cap-bounded), not retained records — 512 retained records still map to ≤128 spans.
  • Noted PR Bound Cosmos diagnostics under retry storms #4683's new coherence guarantee in §7.3: the retained attempts and the per-run rollup are drawn from the same cap-bounded set, so an emitter never encounters a retained attempt whose run was omitted.

Documentation only.

@NaluTripician
NaluTripician marked this pull request as ready for review July 6, 2026 20:07
Copilot AI review requested due to automatic review settings July 6, 2026 20:07
@NaluTripician
NaluTripician requested a review from a team as a code owner July 6, 2026 20:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This documentation-only PR adds DIAGNOSTICS-CONTRACT.md to the azure_data_cosmos_driver crate. It specifies the driver↔SDK diagnostics contract and its OpenTelemetry mapping as a standalone design document, deliberately decoupled from the deferred capture-engine prototype (#4619). It defines a cheap always-available handle with explicit, lazy materialization into three representations (structured metrics / OTel spans / JSON string), an opaque-handle FFI boundary for future Go/Python SDKs, a DiagnosticsLevel gating knob that bounds only transport-level telemetry, a bounded-size guarantee for retry storms, and the concrete OTel metric/trace/attribute mapping aligned with azure_core.

Changes:

  • Adds a new design doc describing the diagnostics contract, FFI boundary, level/threshold gating, and bounded-size guarantee.
  • Documents the OTel metrics + traces mapping and azure_core attribute alignment (including the az.service_request.id dot and private-constant caveats).
  • Records resolved design decisions (handle/materialize surface, FFI lifetime, gating enum, caps, SDK-vs-driver emission split, aggregated span shape); no code or public API changes.

Comment thread sdk/cosmos/azure_data_cosmos_driver/DIAGNOSTICS-CONTRACT.md Outdated
Comment thread sdk/cosmos/azure_data_cosmos_driver/DIAGNOSTICS-CONTRACT.md Outdated
Address the Copilot review comments and unblock the documentation-only
CI checks on PR Azure#4684:

- Reword the two "Shipped today (PR Azure#4683)" claims (in the bounded-size
  section and the Q4 row) to "Proposed in PR Azure#4683 (not yet on `main`)".
  `max_request_diagnostics` is added by the unmerged Azure#4683, so this
  stays consistent with the "standalone on `main`" framing and no longer
  implies a reader will find the symbol via the `[opts]` link, which on
  `main` exposes only `max_summary_size_bytes` and `default_verbosity`.
- Convert the five relative source-file reference links to absolute
  GitHub URLs so the link verification check passes.
- Add the doc's technical terms (flatbuffer, materializer(s),
  uncompacted, underspecified) to the cSpell dictionary.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@NaluTripician

Copy link
Copy Markdown
Contributor Author

Pushed e246b0d2f to address the review feedback and the documentation-only CI failures:

  • Review comments (§6 + Q4): reworded the two "Shipped today (PR Bound Cosmos diagnostics under retry storms #4683)" claims to "Proposed in PR Bound Cosmos diagnostics under retry storms #4683 (not yet on main)", since max_request_diagnostics is added by the unmerged Bound Cosmos diagnostics under retry storms #4683 and isn't in diagnostics_options.rs on main (which only exposes max_summary_size_bytes / default_verbosity).
  • Verify Links / Link verification check: the 5 broken relative ./src/... reference links are now absolute GitHub URLs. Verified locally with eng/common/scripts/Verify-Links.ps1 — no broken links.
  • cspell (Build Analyze): added the doc's technical terms (flatbuffer, materializer(s), uncompacted, underspecified) to .vscode/cspell.json. cspell clean locally.

Build Pack Crates is a pre-existing, workspace-wide failure that this documentation PR doesn't introduce and can't fix: cargo package -p azure_data_cosmos_driver fails because the crate's azure_identity dev-dependency resolves to the unpublished 1.1.0-beta.1. The sibling code PR #4683 hits the identical Build Pack Crates failure. Adding the doc into the crate directory simply puts the crate in the "changed" set that triggers the pack step.

@analogrelay analogrelay left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a contract between SDK and Driver, I think this generally looks good. I think we'll want to think carefully about the collection logic, but that can come next. The key thing is that idea of thresholds being used to gate materialization rather than collection, since we need to be able to evaluate them after the request completes.

Comment thread .vscode/cspell.json
@github-project-automation github-project-automation Bot moved this from Todo to Changes Requested in CosmosDB Rust SDK and Driver Jul 8, 2026
Move the five Cosmos-specific spellings (flatbuffer, materializer, materializers, uncompacted, underspecified) out of the global .vscode/cspell.json and into the Cosmos-scoped sdk/cosmos/.cspell.json ignoreWords list, per reviewer feedback from @heaths. These terms are only used by sdk/cosmos/azure_data_cosmos_driver/DIAGNOSTICS-CONTRACT.md, so scoping them keeps the global dictionary free of Cosmos-only words. The global config is now identical to its pre-PR state.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolved sdk/cosmos/.cspell.json by unioning main's new ignoreWords with the five Cosmos diagnostics terms scoped in this PR (flatbuffer, materializer, materializers, uncompacted, underspecified).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@heaths heaths left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cspell-related changes look good. I didn't review the other files.

@github-project-automation github-project-automation Bot moved this from Changes Requested to Approved in CosmosDB Rust SDK and Driver Jul 14, 2026
NaluTripician added a commit to NaluTripician/azure-sdk-for-rust that referenced this pull request Jul 15, 2026
Salvage the DIAGNOSTICS-CONTRACT.md skeleton from PR Azure#4684 onto a fresh main-based branch and fix its substantive errors:

- Correct the OpenTelemetry semconv names to the real ones: db.client.operation.duration, db.response.status_code, db.operation.name, azure.cosmosdb.* (request_charge, consistency.level, response.sub_status_code, active_instance.count) and db.system.name=azure.cosmosdb, replacing the non-existent db.cosmosdb.* names.
- Add the DiagnosticsHandler chain as the SDK emission model (D1).
- Add tail-based sampling as a concept distinct from DiagnosticsLevel (D4).
- Add cross-operation rate limiting (D5), distinct from the per-artifact size bound.
- Note the azure_core metrics/backdating gaps and the develop-local-then-upstream plan (D2/D3).
- Re-tag DiagnosticsContext and friends as WS0 (not on main yet); only DiagnosticsThresholds is actually on main today.
- Add an R1-R9 requirement-to-section map.

Documentation only; no code changes and no CHANGELOG entry. Spelling and markdownlint pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@analogrelay

Copy link
Copy Markdown
Member

@NaluTripician ready to merge?

@NaluTripician

Copy link
Copy Markdown
Contributor Author

Superseded by #4789. The diagnostics contract/OpenTelemetry-mapping doc was corrected (real semconv names, handler-chain emission model, tail-based sampling, rate-limiting, and the [main] vs new-work tags fixed to reflect that the DiagnosticsContext foundation is already on upstream) and folded into #4789 as DIAGNOSTICS-CONTRACT.md alongside the implementation it describes. Closing this design-only prototype.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Cosmos The azure_cosmos crate

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants